Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: introduce use_did and use_did_method #2862

Merged
merged 23 commits into from
Apr 8, 2024
Merged

refactor: introduce use_did and use_did_method #2862

merged 23 commits into from
Apr 8, 2024

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Apr 1, 2024

This implements #2857. With one modification: rather than having a single parameter use_did, I found that the processing became significantly simpler by having a use_did and use_did_method parameter. Hopefully that's an acceptable trade off.

WIP: Currently, just the updates to the did exchange routes have been made. Working through the OOB route updates now. Wanted to open early for feedback.

@swcurran swcurran requested review from ianco and amanji April 1, 2024 20:30
amanji
amanji reviewed Apr 1, 2024
View reviewed changes
Copy link
Contributor

@amanji amanji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far. Some nice cleanup

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 2, 2024 •
edited
Loading

Quick update, I took a stab at wrangling the logic of the create_invitation method on the OOB manager by introducing a separate class for the purpose. Felt like overkill but the more I leaned into the approach, the clearer it became to me that the logic is rather complex and having it be split up this way might help. Interested in feedback if anyone has any.

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 3, 2024

https://github.com/hyperledger/aries-cloudagent-python/actions/runs/8542753211/job/23405071552?pr=2862#step:3:31413

I think this test failure is unrelated so I'll focus on fixing unit tests first and removing the emit flags officially.

@swcurran
Copy link
Contributor

swcurran commented Apr 3, 2024

Yes — we are getting this error too often. Pretty sure it is that the DID Seed is not random enough — we keep reusing DIDs used in earlier tests...

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 4, 2024

Quick note that with the removal of the --emit-did-peer-4 and --emit-did-peer-2 flags, I believe that would mean that the AATH tests recently added for qualified DIDs might need to be updated since those tests depend on using these flags.

@dbluhm dbluhm force-pushed the refactor/use-did branch 3 times, most recently from 768966e to 6c2d2b2 Compare April 4, 2024 02:31
@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 4, 2024

I was seeing test failures that I was not certain were related to my changes here so I went ahead and fixed #2855 in 62744c1

@dbluhm dbluhm force-pushed the refactor/use-did branch from d109820 to 13f6cd3 Compare April 4, 2024 16:02
@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 4, 2024

Interop testing is looking pretty good. I have some small issues to sort out with backwards compat still (my respond-in-kind for the message version is a little wonky) but should be wrapping up soon.

dbluhm added 17 commits April 4, 2024 15:54
@dbluhm
refactor: introduce use_did and use_did_method
239ba8f 
To didexchange admin api routes that trigger an outbound didexchange
request

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
feat: oob route updates for use_did, use_did_method
15f3e10 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: comment out create_unique_did for now in int tests
e61e709 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
feat: remove emit args
4da0cb6 
But add emit args to demo/int test args

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
refactor: delegate param checking to invite creator
ccd92d8 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: use_did_method is body param
c2b1b80 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: use_did_method should be none when not set
b15407a 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: omit use_did_method when none
e9f6e7b 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: condition for create_unique_did
a03db24 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: more random dids
a52cf65 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: move emit flag in issue cred test defs
c020a79 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
feat: don't remove type info from agent message on deserialize
92f0187 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: conductor invite creation tests
eb7ee17 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: agent message tests with bad type
30fea83 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: route manager test with updated params to route_verkey
5228382 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: remove out of spec attr for invitation message tests
a2b69be 
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
fix: oob manager test failures
7f5eae7 
Corrected serialization, service decorator should be base58 raw keys

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm force-pushed the refactor/use-did branch from ba3943f to ff8a613 Compare April 4, 2024 19:54
@dbluhm dbluhm mentioned this pull request Apr 4, 2024
Closed
@dbluhm dbluhm marked this pull request as ready for review April 4, 2024 20:15
@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 4, 2024

This is ready for review!

ianco
ianco previously approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Are there any docs we need to update?

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 8, 2024

Good point! I'll see if I can toss in a quick doc update somewhere

This was linked to issues Apr 8, 2024
Updates to enable the backwords compatibility and transitioning to did:peer:2/4 and all qualified DIDs #2857
Closed
Integration tests creating existing DID's #2855
Closed
@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 8, 2024

As I was writing the docs, I realized there is one case I overlooked: when the inviter creates an invite with didexchage/1.1 only and the invitee supports 1.1, it is still sending an unqualified DID in a 1.1 message if the invitee accepts the invitation without specifying a use_did_method. I believe it would be preferable for the invitee to automatically prefer using did:peer:4 in this case. I'll go ahead and make that tweak but open to thoughts if there are any.

jamshale
jamshale previously approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dbluhm
fix: a few critical tweaks
fd73dd6 
- on 1.1, default to use_did_method=did:peer:4 when not specified
- on use_did, fix bugs with setting the did
- doc updates

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm dismissed stale reviews from jamshale and ianco via fd73dd6 April 8, 2024 17:52
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Apr 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 8, 2024 •
edited
Loading

@ianco @jamshale I made a couple of fixes and added a new doc outlining use of qualified DIDs in ACA-Py, with details on OOB, DID Exchange, and DID Rotation.

@swcurran
Copy link
Contributor

swcurran commented Apr 8, 2024

@dbluhm — are you happy that this is ready to merge? If so — we can push 0.12.0-rc3 out.

@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 8, 2024

Yep, ready! I was half keeping an eye on the integration tests just to be sure but my testing indicates that it should be fine

@swcurran swcurran merged commit a9e0cf7 into openwallet-foundation:main Apr 8, 2024
8 checks passed
@dbluhm dbluhm deleted the refactor/use-did branch April 8, 2024 19:33
@nodlesh nodlesh mentioned this pull request Apr 15, 2024
Merged
@dbluhm dbluhm mentioned this pull request Apr 16, 2024
Closed
@ff137 ff137 mentioned this pull request Apr 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amanji amanji amanji left review comments

@jamshale jamshale jamshale approved these changes

@ianco ianco ianco approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@dbluhm @swcurran @ianco @amanji @jamshale